AnchoredOverlayTestComponent is spreading the overlayProps test setting directly onto <AnchoredOverlay> ({...overlayProps}), which passes disablePortal as a top-level prop instead of as overlayProps={...}. This should be passed via the overlayProps prop (e.g. overlayProps={overlayProps}), otherwise TS should reject it and the test won't be exercising the intended behavior.

I couldn't find data-anchor used anywhere in the CSS, is this for future styles?

You're right! We used to use it, but I removed it in a previous commit. I can go ahead and remove it!

Curious about this part. Is this because when a Portal remounts, it creates a new Overlay element and :popover-open is lost on this new element?

Yes exactly! In steps this is how it works:

• The user clicks the trigger button for AnchoredOverlay which turns open to true
• We check if the overlay ref is not null (if it's null then the Overlay hasn't rendered yet)
• We then check if it's open (along with other conditionals such as if the feature flag is enabled)
• Lastly, we check if it's somehow open already, as we're using the popover api, we can do this by checking if :popover-open is present. This guards us from re-opening the overlay multiple times whenever one of the dependencies change.

Let me know if this makes sense, or if you have questions!

Makes sense, and it's clearer after reading the conversation you and @siddharthkp had, thanks!

curious, why do we need important here? Are there any overrides in dotcom that this might break?

There was a case where position was being overridden (example: https://github.com/github/github-ui/pull/16228). The !important will most likely be temporary, as we can probably make the selector take precedence in regards to specificity.

Not sure if we should make this prop part of the public API. Do we want folks to use it at an instance level? Should this be an invisible detail of using the feature flag?

Yeah that's fair! This one is more so to test when/if we need the portal. Most of the cases where a portal came in handy was with styling. Any styling that the parent had would leak into the AnchoredOverlay, which the portal prevents.

I'll adjust this so that it's only useable with the flag along with making it "private".

Curious about this comment because we seem to only want to use the ref when using css anchor positioning and popover, so not using a portal. Am I reading this wrong?

You're right. I'll adjust the comment 😅

This is trying to say that if the overlay is mounted/unmounted the popover itself is no longer open (via :popover-open). When it is mounted or re-rendered, we need to open it manually again via showPopover (if open is true) which we need the active ref for. Let me know if this makes sense or not!

Ohhh okay. I understand this in theory, when does this happen in prod?

I was running into an issue with Issues SelectPanel, where it would render the SelectPanel only after the initial click. It would be within the DOM but it wouldn't be opened as expected.

On first click, the component would mount and attach its ref. The useLayoutEffect runs with the new DOM node, checks that it's not already opened (via :popover-open), and calls showPopover() to actually display it. Without the effect, the element would exist in the DOM with but never be shown.

Since we're using popover='manual' we need to explicitly use showPopover() to display the popover. The effect ensures we call it whenever a new DOM node is created, whether from the initial open, or from dynamic/lazy mounting where the element gets recreated (such as Issues SelectPanel).

Let me know if I can go into more details too! I'll see about adding context on the Issues SelectPanel case which made me layer the effect this way in https://github.com/github/primer/issues/6321!